-
Notifications
You must be signed in to change notification settings - Fork 192
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
adding AI project #944
adding AI project #944
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very small nit comments
src/DotNetWorker.ApplicationInsights/FunctionsApplicationInsightsExtensions.cs
Outdated
Show resolved
Hide resolved
src/DotNetWorker.ApplicationInsights/FunctionsTelemetryInitializer.cs
Outdated
Show resolved
Hide resolved
src/DotNetWorker.ApplicationInsights/FunctionsTelemetryInitializer.cs
Outdated
Show resolved
Hide resolved
/// <summary> | ||
/// Gets or sets a value indicating whether to send <see cref="ILogger"/> logs through the Functions host. | ||
/// </summary> | ||
public bool DisableHostLogger { get; set; } = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Struggling with this becoming a public property on the worker options, as the host logger is a gRPC specific concept, but I really don't have a great alternative at the moment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One of the things I had previously mentioned in discussion would be to introduce a proper service for user logs. By default, the gRPC implementation would register one (overridable) that would push to the host. If the implementation here registers one as well, that would bypass the gRPC logic without requiring a flag. Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let me look at that -- will submit a change here soon with this split out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm! Just 1 question about custom metrics and use of a const file :D
src/DotNetWorker.ApplicationInsights/FunctionsApplicationInsightsExtensions.cs
Outdated
Show resolved
Hide resolved
@@ -0,0 +1,94 @@ | |||
using System; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit - Are there certain files that need the copyright header and some that don't?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No they all should... will go through and double-chedk!
namespace Microsoft.Azure.Functions.Worker.Logging | ||
{ | ||
/// <summary> | ||
/// Minimalistic LogWriter that does nothing. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you elaborate on this LogWriter? Trying to understand it. Is it for when a user doesn't create/define other specific loggers (UserLogWriter, SystemLogWriter) and this is the default put in their place?
This summary caught me off guard 🤣
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All of our "logging through the host" goes through Grpc, which isn't part of the core packages. So we need some default way to basically "do nothing" without breaking dependency injection if for some reason Grpc isn't wired up. In reality this will be very rare.
return async context => | ||
{ | ||
var middleware = context.InstanceServices.GetRequiredService<FunctionActivitySourceMiddleware>(); | ||
await middleware.Invoke(context, next); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible that the customer can register a middleware before calling the AddApplicationInsights
method in their Program.cs, which will cause the this middleware to be executed only after the customer middleware and any logging coming from customer m/w will miss the code from FunctionActivitySource.StartInvoke
? I am guessing when we move this to Core project eventually, that may not be a problem as we can register this m/w before customer m/w?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that's why we'll move this behavior to the Core bits before releasing. It won't actually be middleware in the end, we'll put it directly into the FunctionsApplication.InvokeFunctionAsync()
so that it's (effectively) un-overrideable.
src/DotNetWorker.ApplicationInsights/FunctionsApplicationInsightsExtensions.cs
Show resolved
Hide resolved
{ | ||
if (WorkerMessage.IsSystemLog) | ||
{ | ||
_systemLogWriter.WriteSystemLog(_scopeProvider, _category, logLevel, eventId, state, exception, formatter); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would this log anything ? Because ISystemLogWriter
implementation registered to DI container is NullLogWriter
which does nothing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't we ideally call some APIs on TelemetryClient to send this to AI?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This gets overridden by the call to AddGrpc (it adds the GrpcFunctionsHostLogWriter
via DI).
System logs are meant to be only for us (in kusto). They have to go back through the host and don't show up in App Insights today. That's why I had to split things out so weirdly -- we needed ways to stop one type of log (user logs) from going through the host, while allowing others (the system logs) to continue flowing.
services.AddSingleton<IUserLogWriter>(p => p.GetRequiredService<GrpcFunctionsHostLogWriter>()); | ||
services.AddSingleton<ISystemLogWriter>(p => p.GetRequiredService<GrpcFunctionsHostLogWriter>()); | ||
services.AddSingleton<IUserMetricWriter>(p => p.GetRequiredService<GrpcFunctionsHostLogWriter>()); | ||
services.AddSingleton<IWorkerDiagnostics, GrpcWorkerDiagnostics>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unrelated to changes from this PR:
Are we supposed to call _outputChannel.TryWrite(message);
here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Certainly seems like it... I can follow up on that with a separate PR since it's unrelated.
builder.Services.TryAddEnumerable(new ServiceDescriptor(typeof(ITelemetryInitializer), typeof(FunctionsTelemetryInitializer), ServiceLifetime.Singleton)); | ||
builder.Services.TryAddEnumerable(new ServiceDescriptor(typeof(ITelemetryModule), typeof(FunctionsTelemetryModule), ServiceLifetime.Singleton)); | ||
|
||
// User logs will be written directly to Application Insights; this prevents duplicate logging. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We also register GrpcFunctionsHostLogWriter
in DotNetWorker.Grpc/GrpcServiceCollectionExtensions.cs. So that is going to send logs to host (duplicate). Correct?
.ConfigureFunctionsWorkerDefaults(builder => | ||
{ | ||
builder | ||
.AddApplicationInsights() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we consider exposing a single method (which internally calls these 2) for the convenience of user?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally yes... but App Insights has these as two separate concepts. I'd like to try to mirror their usage as much as possible so we can offload the documentation to them. I agree it's kinda odd... but we can review before making this GA.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's get this in so we can start validating the functionality. We'll continue to iterate as you go.
@brettsam You need to add a dependency on |
For me this works in general but I still have difficulties getting logs from the isolated worker with a level lower than Warning.
appsettings.json has the same settings like host.json which work perfectly for the host. Inside the worker I tried logging to a ILoggerFactory via DI and also via the FunctionContext.GetLogger().
The first one comes through, the second one not. |
@Done3319 try #1123 (comment) (or #1123 (comment)) |
Hello, I was trying to used isolated function to run Availability test. But i already new this package and discarded it in the past for other function as i needed scope logs and its hardcoded in src/DotNetWorker.ApplicationInsights/FunctionsApplicationInsightsExtensions.cs My question is then (probably @brettsam) is there a reason why this is harcoded? |
Hi @brettsam, is there a reason why TelemetryClient isn't registered as singleton? Doing so would allow us to resolve that dependency and start tracking events. The scenario I see is to track retries and single successful runs for Azure Service Bus triggers. Right now https://github.com/Azure/azure-functions-dotnet-worker/blob/main/src/DotNetWorker.ApplicationInsights/FunctionsTelemetryModule.cs creates a TelemetryClient. I see no reason why this dependency can't be registered as singleton. https://github.com/Azure/azure-functions-dotnet-worker/blob/main/src/DotNetWorker.ApplicationInsights/FunctionsApplicationInsightsExtensions.cs alsready registers the module as singleton. Or are you worried that someone already registered the TelemetryClient and that's why you would like an instance of "your own"? |
We're doing nothing special with App Insights types or registrations. We call As for why the module uses its own client -- I was following the patterns of other modules. (https://github.com/microsoft/ApplicationInsights-dotnet/blob/main/WEB/Src/Web/Web/ExceptionTrackingTelemetryModule.cs) I seem to recall that during testing this, the modules have some circular dependency issues and you can't inject the client there. I think it's b/c the |
Yes, I discovered that this afternoon: once I include and enable the Functions AI package, I can resolve the TelemetryClient. |
@brettsam given this is merged, what are the plane to actually release the |
Hi @brettsam , My Program.cs is defined as:
and our packages are:
Would you have an idea if I need to add some extra config or something else? |
This PR introduces a new nuget package (in preview):
Microsoft.Azure.Functions.Worker.ApplicationInsights
. This package should help to address issues like #702, #760, and others.The goal of this new package is to simplify the Application Insights wire-up required in order to have proper correlation between the RequestTelemetry generated in the Functions host and any telemetry generated during the invocation on the isolated worker.
The package uses as much of the default Application Insights setup as possible so that we can utilize the existing documentation and (hopefully) not introduce many new Functions-only concepts.
Background
Azure Functions has automatic support for App Insights built into the host (and for in-proc .NET functions). This emits
RequestTelemetry
for every invocation,TraceTelemetry
forILogger
logs, has Live Metrics enabled and auto-tracks Dependencies like outgoing HTTP calls, SQL queries, etc.In the out-of-proc worker model, a gRPC connection is made between the worker and host. This connection is used to send invocation details from the host to the worker, but it’s also used for a worker to send log messages back to the host. This allows a worker to expose some logger class that behind-the-scenes sends log messages to the host via gRPC, which then logs it to the configured Application Insights resource through the host. This is the extent of "Application Insights support" that we offer to all of the out-of-proc workers. Side note that this support only works during an invocation. Any logs emitted outside the scope of an invocation are ignored by the host, which results in issues like #702.
For the dotnet-isolated model we want to expand this support to create a richer offering that allows the worker to emit telemetry directly to Application Insights while maintaining the proper telemetry correlation (i.e. Dependencies are auto-tracked and show up as part of the overall Request).
The package introduces two new extension methods available on
IFunctionsWorkerApplicationBuilder
that allow you register Application Insights services (AddApplicationInsights()
)and the Application Insights logger (AddApplicationInsightsLogger()
).AddApplicationInsights()
This method internally calls the Application Insights
IServiceCollection
extension methodAddApplicationInsightsTelemetryWorkerService()
, with a couple of extra services added to provide the correlation needed for Functions. See the Application Insights documentation for Worker Services for details. The Functions extension method includes an optional callback parameter to allow you to configure the underlyingApplicationInsightsServiceOptions
options.With the default configuration, this will include:
Azure.Functions
with nameInvoke
that tracks the time spent on the isolated worker.AddApplicationInsightsLogger()
This method internally calls the Application Insights
ILoggingBuilder
extension methodAddApplicationInsights()
, with a couple of extra services added to provide the correlation needed for Functions. See the Application Insights documentation for using ILogger in a Console application for details. The Functions extension method includes an optional callback parameter to allow you to configure the underlyingApplicationInsightsLoggerOptions
options.Note that calling this function will disable the gRPC logging that occurs by default; all
ILogger
logs will be emitted directly to Application Insights asTraceTelemetry
. This should address issues like #702 without changing host behavior and affecting other language workers.Configuration
Some notes on configuration:
APPLICATIONINSIGHTS_CONNECTION_STRING
will automatically wire up the both the host and the worker to the same Appliation Insights instance.host.json
. For example, if you've disabled sampling in the host, you will need to also disable it in the worker. Same with configuring logging levels. Please refer to the documentation links above to configure your worker.